Skip to content

fix: SentinelNodeFinder uses now mangled_name#95

Merged
alexmojaki merged 4 commits intomasterfrom
fix_mangled_name
Aug 20, 2025
Merged

fix: SentinelNodeFinder uses now mangled_name#95
alexmojaki merged 4 commits intomasterfrom
fix_mangled_name

Conversation

@15r10nk
Copy link
Collaborator

@15r10nk 15r10nk commented Jan 14, 2025

closes #99

@15r10nk 15r10nk mentioned this pull request Aug 17, 2025
@15r10nk 15r10nk marked this pull request as ready for review August 17, 2025 15:11
Copy link
Owner

@alexmojaki alexmojaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks so much

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Comment on lines 548 to 549
if sys.version_info[0] == 3 or instruction.argval:
extra_filter =lambda e:mangled_name(e) == instruction.argval
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if sys.version_info[0] == 3 or instruction.argval:
extra_filter =lambda e:mangled_name(e) == instruction.argval
extra_filter =lambda e:mangled_name(e) == instruction.argval

python 3 only now

Comment on lines 575 to 578
print(instruction)

print(exprs)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print(instruction)
print(exprs)

import dis

def test_mangled_name():
@pytest.mark.skipif(sys.version_info < (3,), reason="argval of some Instructions is None")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@pytest.mark.skipif(sys.version_info < (3,), reason="argval of some Instructions is None")

Comment on lines 160 to 164
if sys.version_info < (3,):
def indent(s,prefix):
return prefix + s.replace("\n","\n"+prefix)
else:
from textwrap import indent
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if sys.version_info < (3,):
def indent(s,prefix):
return prefix + s.replace("\n","\n"+prefix)
else:
from textwrap import indent
from textwrap import indent

Comment on lines 157 to 158
from executing._utils import mangled_name
import dis
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls move to top

ast.ExceptHandler,
),
)
if sys.version_info>=(3,):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to check any more

# IMPORT_FROM(_Test__submodule11c)
# STORE_NAME(_Test__subc11)

if instruction.opname=="LOAD_ATTR" and before is not None and before.opname == "IMPORT_NAME":
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if instruction.opname=="LOAD_ATTR" and before is not None and before.opname == "IMPORT_NAME":
if instruction.opname == "LOAD_ATTR" and before is not None and before.opname == "IMPORT_NAME":

we really need proper tooling in this repo like ruff

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I was thinking the same already last year. I will create an issue with some step by step plan.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Aug 17, 2025

Thank you for taking the time to review this, and please excuse the silly mistakes.

typ = ast.Name
ctx = ast.Load
extra_filter = lambda e: e.id == instruction.argval
if instruction.argval:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original was if sys.version_info[0] == 3 or instruction.argval:, with an or, so is this check needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not needed 4 lines above and it is most likely not needed here.

@alexmojaki
Copy link
Owner

any idea what's going on with github?

@15r10nk
Copy link
Collaborator Author

15r10nk commented Aug 18, 2025

Microsoft has taken over ... I have no idea.

@15r10nk
Copy link
Collaborator Author

15r10nk commented Aug 18, 2025

I have restarted them with debug output but they are just waiting ...

@15r10nk
Copy link
Collaborator Author

15r10nk commented Aug 19, 2025

github could not schedule the jobs because ubuntu 20.04 was not available any more.

# STORE_NAME(_Test__subc11)

if instruction.opname == "LOAD_ATTR" and before is not None and before.opname == "IMPORT_NAME":
continue
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a comment to explain

# covered by the verifier and much more complex in python 3.11
if isinstance(node, ast.Name) and not py11:
assert inst.argval == node.id, (inst, ast.dump(node))
if isinstance(node, ast.Name) and inst.opname != "CALL_INTRINSIC_1" and inst.argval not in ("__classdict__","__classdictcell__","__static_attributes__"):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this worth explaining?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting it caused no problem and I can't remember why this was needed. Maybe it is just not needed any more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this reply was meant to be on the other comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry. I wrote it on my phone.

@alexmojaki alexmojaki merged commit f64f00d into master Aug 20, 2025
7 checks passed
@alexmojaki
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test collection error with pytest 8.4.1

3 participants